refactor: host alias logic in wrapper by karenc-bq · Pull Request #431 · aws/aws-advanced-jdbc-wrapper

karenc-bq · 2023-05-05T18:49:58Z

Summary

Integration Test Run

https://github.com/Bit-Quill/aws-advanced-jdbc-wrapper/actions/runs/4920404497

Description

Issues fixed

After failover Aurora Connection Tracker incorrectly invalidates the newly promoted writer
After failover Aurora Connection Tracker incorrectly invalidates a random reader that is neither the old or new writer.
If the initial connection is established using an cluster endpoint, EFM plugin uses the cluster endpoint as the monitoring endpoint

Issue 1 Solution

Issue one is caused by the following check.

  private boolean isRoleChanged(final EnumSet<NodeChangeOptions> changes) {
    return changes.contains(NodeChangeOptions.PROMOTED_TO_WRITER)
        || changes.contains(NodeChangeOptions.PROMOTED_TO_READER);
  }

We need to remove the check for PROMOTED_TO_WRITER

Issue 2 & 3

Issues 2 and 3 are both caused by missing or incorrect instance endpoint in the hostspec's aliases. Issues 2 and 3 were originally fixed in #428 and #428, and the fixes contain a lot of similarities. This refactoring centralizes the logic populating the aliases and make it less error prone.

More information in respective tickets:
https://github.com/orgs/awslabs/projects/91/views/13?pane=issue&itemId=27190320
https://github.com/orgs/awslabs/projects/91/views/13?pane=issue&itemId=27190427

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

wrapper/src/main/java/software/amazon/jdbc/PluginServiceImpl.java

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

sergiyvamz · 2023-05-05T19:13:58Z

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

+      if (resultSet.next()) {
+        final String instanceName = resultSet.getString(1);
+        if (this.getCachedTopology() == null) {
+          final HostSpec host = new HostSpec(getHostEndpoint(instanceName));


Can we refresh topology with provided connection?

return null;

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

sergiyvamz · 2023-05-05T19:15:53Z

...er/src/main/java/software/amazon/jdbc/hostlistprovider/ConnectionStringHostListProvider.java

+
+  @Override
+  public HostSpec identifyConnection(Connection connection) throws SQLException {
+    return this.hostList.get(0);


return null?

As discussed, we need to get sql from dialect, fetch node id, try to find it in topology.

sergiyvamz · 2023-05-05T19:17:31Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/HostMonitoringConnectionPlugin.java


    if (conn != null) {
-      generateHostAliases(driverProtocol, conn, hostSpec);
+      hostSpec.resetAliases();


Why to reset? Some other plugin might already filled aliases for this host spec.

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorServiceImpl.java

… instead of instance endpoints during status checks.

wrapper/src/main/java/software/amazon/jdbc/HostSpec.java

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/HostMonitoringConnectionPlugin.java

sergiyvamz · 2023-05-05T23:27:27Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorServiceImpl.java

              "MonitorServiceImpl.emptyAliasSet",
              new Object[] {hostSpec}));
-      hostSpec.addAlias(hostSpec.asAlias());
+      try {


I think we can remove this entire try-catch block. We just rely on provided nodeKeys, and if they empty then we raise an exception.

sergiyvamz · 2023-05-06T19:03:22Z

wrapper/src/main/resources/messages.properties

 PluginServiceImpl.hostAliasNotFound=Can''t find any host by the following aliases: ''{0}''.
 PluginServiceImpl.hostsChangelistEmpty=There are no changes in the hosts' availability.
+PluginServiceImpl.failedToRetrieveHostPort=Could not retrieve Host:Port for connection.
+PluginServiceImpl.nonEmptyAliases=fillAliases called when HostSpec already contains the following aliases: ''{0}''. Please reset HostSpec aliases before calling this method.


"Please reset HostSpec aliases before calling this method."

I think it may mislead developers.
"Please reset HostSpec aliases if you need to re-fill them again." ?

…ion after failover

…ion map

Track connections by identityHashCode to avoid re-running alias queries when the same physical connection is reused (e.g., with internal pooling). - Add aliasedConnections set to track already-aliased connections - Only call resetAliases() + fillAliases() on first use of each connection - Clean up tracking on connection close/abort to prevent memory leaks - Add null-check for currentConnection in cleanup path - Add test for cleanup-and-reinitialize behavior This preserves the original PR aws#431 fix (fresh aliases for new physical connections to cluster endpoints) while avoiding 4-5 extra SQL queries per getConnection() when using HikariPooledConnectionProvider. Fixes performance regression with customEndpoint + initialConnection plugins.

refactor: host alias logic in wrapper

f9d4cc0